Skip to content

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Aug 10, 2025

Additionally, this comment has been addressed.

@hebasto
Copy link
Member Author

hebasto commented Aug 10, 2025

FWIW, on Ubuntu 24.04, when using GCC 13.3.0 and gcovr 7.0, I need to pass an extra --merge-mode-functions=separate option to generate a coverage report.

@real-or-random real-or-random added assurance meta/development processes, conventions, developer documentation, etc. labels Aug 11, 2025
@real-or-random
Copy link
Contributor

FWIW, on Ubuntu 24.04, when using GCC 13.3.0 and gcovr 7.0, I need to pass an extra --merge-mode-functions=separate option to generate a coverage report.

Let's add it, and I also need --gcov-suspicious-hits-threshold=140737488355330 (because we hit some lines in int128_impl.h more than 2^32 times...) but this was introduced only in gcovr 8.3. I assume older versions error out when they get this flag?

Otherwise, commands fail with the error:
```
Stderr of gcov was >><< End of stderr
Exception was >>Got function secp256k1_scalar_split_lambda on multiple lines: 67, 142.
	You can run gcovr with --merge-mode-functions=MERGE_MODE.
	The available values for MERGE_MODE are described in the documentation.<< End of stderr
```
@hebasto hebasto force-pushed the 250810-autotools-coverage branch from b1b8869 to 1aecce5 Compare August 11, 2025 10:36
@hebasto
Copy link
Member Author

hebasto commented Aug 11, 2025

FWIW, on Ubuntu 24.04, when using GCC 13.3.0 and gcovr 7.0, I need to pass an extra --merge-mode-functions=separate option to generate a coverage report.

Let's add it...

Thanks! Done.

... and I also need --gcov-suspicious-hits-threshold=140737488355330 (because we hit some lines in int128_impl.h more than 2^32 times...) but this was introduced only in gcovr 8.3. I assume older versions error out when they get this flag?

Yes, it outputs:

usage: gcovr [options] [search_paths...]
gcovr: error: unrecognized arguments: --gcov-suspicious-hits-threshold=140737488355330

@real-or-random
Copy link
Contributor

Yes, it outputs:

usage: gcovr [options] [search_paths...]
gcovr: error: unrecognized arguments: --gcov-suspicious-hits-threshold=140737488355330

Then I guess the best thing for now is to add a note/comment that this should be omitted on gcovr before 8.3.

@hebasto
Copy link
Member Author

hebasto commented Aug 11, 2025

FWIW, on Ubuntu 24.04, when using GCC 13.3.0 and gcovr 7.0, I need to pass an extra --merge-mode-functions=separate option to generate a coverage report.

Let's add it, and I also need --gcov-suspicious-hits-threshold=140737488355330 (because we hit some lines in int128_impl.h more than 2^32 times...) but this was introduced only in gcovr 8.3. I assume older versions error out when they get this flag?

What GCC version are you using? Any other specific steps to reproduce the issue?

EDIT: nm, I can reproduce it on Fedora 42 with GCC 15.1.1.

@real-or-random
Copy link
Contributor

I think an alternative for <8.3 is --gcov-ignore-parse-errors=suspicious_hits.warn, which should work on >=6.0 if I understand the changelog correctly.

Fwiw:

gcovr 8.4.dev0+gfe536afac.d20250125
gcc (GCC) 15.1.1 20250729

@hebasto
Copy link
Member Author

hebasto commented Aug 11, 2025

I think an alternative for <8.3 is --gcov-ignore-parse-errors=suspicious_hits.warn, which should work on >=6.0 if I understand the changelog correctly.

It was introduced in gcovr/gcovr#898 and has been available since version 8.0.

The --gcov-ignore-parse-errors=all option can be used instead.

@hebasto
Copy link
Member Author

hebasto commented Aug 11, 2025

The --gcov-ignore-parse-errors=all option can be used instead.

Taken this:

$ gcovr --gcov-ignore-parse-errors=all --merge-mode-functions=separate --exclude 'src/bench*' --exclude 'src/modules/.*/bench_impl.h' --print-summary
(INFO) Reading coverage data...
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_accum_mul(secp256k1_uint128 *r, uint64_t a, uint64_t b) {'.
(WARNING) Ignoring suspicious hits in line '   *r += (uint128_t)a * b;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE uint64_t secp256k1_u128_to_u64(const secp256k1_uint128 *a) {'.
(WARNING) Ignoring suspicious hits in line '   return (uint64_t)(*a);'.
(WARNING) Ignoring suspicious hits in line '                for (p = 0; p < 16; ++p) { /* p loops over the bit positions in mul2[j]. */'.
(WARNING) Ignoring suspicious hits in line '                for (p = 0; p < 16; ++p) { /* p loops over the bit positions in mul2[j]. */'.
(WARNING) Ignoring suspicious hits in line '                    int bitpos = j * 16 - i + p; /* bitpos is the correspond bit position in m. */'.
(WARNING) Ignoring suspicious hits in line '                    if (bitpos >= 0 && bitpos < 256) {'.
(WARNING) Ignoring suspicious hits in line '                    if (bitpos >= 0 && bitpos < 256) {'.
(WARNING) Ignoring suspicious hits in line '                    if (bitpos >= 0 && bitpos < 256) {'.
(WARNING) Ignoring suspicious hits in line '                        sub |= ((m[bitpos >> 4] >> (bitpos & 15)) & 1) << p;'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_accum_mul(secp256k1_uint128 *r, uint64_t a, uint64_t b) {'.
(WARNING) Ignoring suspicious hits in line '   *r += (uint128_t)a * b;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_rshift(secp256k1_uint128 *r, unsigned int n) {'.
(WARNING) Ignoring suspicious hits in line '   *r >>= n;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE uint64_t secp256k1_u128_to_u64(const secp256k1_uint128 *a) {'.
(WARNING) Ignoring suspicious hits in line '   return (uint64_t)(*a);'.
(INFO) Writing coverage report...
------------------------------------------------------------------------------
                           GCC Code Coverage Report
<snip>

@real-or-random
Copy link
Contributor

lgtm but I suggest adding a note like this:

On gcovr >=8.3, --gcov-ignore-parse-errors=all can be replaced with --gcov-suspicious-hits-threshold=140737488355330.

Then we have a built-in reminder that we can remove the old argument in the future.

Otherwise, commands might fail due to bugs in the `gcov` tool.
@hebasto hebasto force-pushed the 250810-autotools-coverage branch from 9982c5c to 0458def Compare August 12, 2025 08:12
@hebasto
Copy link
Member Author

hebasto commented Aug 12, 2025

lgtm but I suggest adding a note like this:

On gcovr >=8.3, --gcov-ignore-parse-errors=all can be replaced with --gcov-suspicious-hits-threshold=140737488355330.

Then we have a built-in reminder that we can remove the old argument in the future.

Thanks! The note has been added.

Copy link
Contributor

@real-or-random real-or-random left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 0458def

@real-or-random
Copy link
Contributor

@josibake Want to review this (with whatever tool versions you have on your system)?

@josibake
Copy link
Member

Nice! Thanks for adding this, I ran into this recently and added --gcov-ignore-parse-errors=all after some googling. When I run with --gcov-ignore-parse-errors=all with [email protected], I see the following warnings:

gcovr --gcov-ignore-parse-errors=all --merge-mode-functions=separate --exclude 'src/bench*' --exclude 'src/modules/.*/bench_impl.h' --print-summary
(INFO) Reading coverage data...
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_accum_mul(secp256k1_uint128 *r, uint64_t a, uint64_t b) {'.
(WARNING) Ignoring suspicious hits in line '   *r += (uint128_t)a * b;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE void secp256k1_u128_rshift(secp256k1_uint128 *r, unsigned int n) {'.
(WARNING) Ignoring suspicious hits in line '   *r >>= n;'.
(WARNING) Ignoring suspicious hits in line '}'.
(WARNING) Ignoring suspicious hits in line 'static SECP256K1_INLINE uint64_t secp256k1_u128_to_u64(const secp256k1_uint128 *a) {'.
(WARNING) Ignoring suspicious hits in line '   return (uint64_t)(*a);'.
(INFO) Writing coverage report...
------------------------------------------------------------------------------
                           GCC Code Coverage Report
Directory: .
------------------------------------------------------------------------------
<snip>

The warning goes away when running with --gcov-suspicious-hits-threshold=140737488355330, i.e.:

gcovr --gcov-suspicious-hits-threshold=140737488355330 --merge-mode-functions=separate --exclude 'src/bench*' --exclude 'src/modules/.*/bench_impl.h' --print-summary
(INFO) Reading coverage data...
(INFO) Writing coverage report...
------------------------------------------------------------------------------
                           GCC Code Coverage Report
Directory: .
------------------------------------------------------------------------------
<snip>

Also confirmed I got the same coverage report with both flags.

Copy link
Member

@josibake josibake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 0458def

Tested with [email protected] and confirmed I get a warning with the old flag (--gcovr-ignore-parse-errors=all), and that those warnings go away when running with the new flag suggest for gcovr >= 8.3

@real-or-random real-or-random merged commit d599714 into bitcoin-core:master Aug 13, 2025
116 checks passed
@hebasto hebasto deleted the 250810-autotools-coverage branch August 13, 2025 20:06
josibake added a commit to josibake/bitcoin that referenced this pull request Sep 5, 2025
aa85bfb530 docs: update README
9f42a30b82 ci: enable silentpayments module
d504e48145 tests: add sha256 tag test
124750d580 tests: add constant time tests
b35ffa2e30 tests: add BIP-352 test vectors
038c5b9c9d silentpayments: add benchmarks for scanning
88eb3d4545 silentpayments: add examples/silentpayments.c
22b20fd617 silentpayments: receiving
df1de93765 silentpayments: recipient label support
76a0451c76 silentpayments: sending
3cd3a93bff build: add skeleton for new silentpayments (BIP352) module
f36afb8b3d Merge bitcoin-core/secp256k1#1725: tests: refactor tagged hash verification
5153cf1c91 tests: refactor tagged hash tests
d2dcf52091 Merge bitcoin-core/secp256k1#1726: docs: fix broken link to Tromer's cache.pdf paper
489a43d1bf docs: fix broken link to eprint cache.pdf paper
d599714147 Merge bitcoin-core/secp256k1#1722: docs: Exclude modules' `bench_impl.h` headers from coverage report
0458def51e doc: Add `--gcov-ignore-parse-errors=all` option to `gcovr` invocations
1aecce5936 doc: Add `--merge-mode-functions=separate` option to `gcovr` invocations
106a7cbf41 doc: Exclude modules' `bench_impl.h` headers from coverage report
a9e955d3ea autotools, docs: Adjust help string for `--enable-coverage` option
e523e4f90e Merge bitcoin-core/secp256k1#1720: chore(ci): Fix typo in Dockerfile comment
24ba8ff168 chore(ci): Fix typo in Dockerfile comment
74b8068c5d Merge bitcoin-core/secp256k1#1717: test: update wycheproof test vectors
c25c3c8a88 test: update wycheproof test vectors
20e3b44746 Merge bitcoin-core/secp256k1#1688: cmake: Avoid contaminating parent project's cache with `BUILD_SHARED_LIBS`
2c076d907a Merge bitcoin-core/secp256k1#1711: tests: update Wycheproof
7b07b22957 cmake: Avoid contaminating parent project's cache with BUILD_SHARED_LIBS
5433648ca0 Fix typos and spellings
9ea54c69b7 tests: update Wycheproof files

git-subtree-dir: src/secp256k1
git-subtree-split: aa85bfb530b9ffc3dde6eaa7a976e129b8bd2f58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
assurance meta/development processes, conventions, developer documentation, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants